The latest updates on your projects. Learn more about Vercel for Git ↗︎

I see a handful of tests on sui-core

authority::authority_tests::test_dry_run_dev_inspect_dynamic_field_too_new: test
authority::authority_tests::test_dry_run_dev_inspect_max_gas_version: test
authority::authority_tests::test_dry_run_no_gas_big_transfer: test
authority::authority_tests::test_dry_run_on_validator: test
authority::authority_tests::test_dry_run_transaction_block: test
authority::authority_tests::test_for_inc_201_dry_run: test

I'll look at expanding these. Appreciate the feedback I've been comparing the two code paths (dry vs dev inspect) and the only notable difference I found so far was the return types but more tests will be good as well.

the issue I have and would love to see discussed (thus the tagging I added) is that - as I remember - devinspect performs operations that we do not do in dryrun, for instance saving results per command. Also I am not clear how dev inspect is going to evolve in the face of transaction replay and the ability to get more insights in a transaction as opposed to normal execution.
I think of dry run as an execution that has got high fidelity with normal execution and the idea of conflating that with dev inspect seems a stretch to me.
And I believe that is deeper than just the name and it goes into the fact I could make change to dev inspect not realizing I am now affecting dry run.
In other words I would love to understand how other people (particularly those that have thought about that space for much longer than me) see the relationship between dev inspect and dry run, now and moving forward.
I may be also looking at this the wrong way so please correct me if so

Also this PR would bring JSON-RPC in line with how GraphQL works.
https://github.com/MystenLabs/sui/blob/main/crates/sui-graphql-rpc/src/types/query.rs#L179
there appears to be an DevInspect.into() -> DryRunResponse so it might be good to do the swap to devinspect call in the api layer?

I think the performance concerns are interesting and worth digging into. Re: dryRun vs devInspect, the main thing for me is what Jordan mentioned above -- GraphQL implements dryRunTransactionBlock via a devInspect call that has the option to skip checks or not (i.e. it combines the two endpoints, but introduces a parameter), so in GraphQL, this conflation already exists.

The reason to do this was to give us a way to expose richer errors during dry-run (for PTBs in particular), by building on top of ExecutionMode -- the thinking was that most transactions go through a dry-run before they execute, so this is a good way to expose people to higher quality errors, but curious to hear thoughts from the cc'ed folks.

I think the performance concerns are interesting and worth digging into. Re: dryRun vs devInspect, the main thing for me is what Jordan mentioned above -- GraphQL implements dryRunTransactionBlock via a devInspect call that has the option to skip checks or not (i.e. it combines the two endpoints, but introduces a parameter), so in GraphQL, this conflation already exists.

Could you explain more what "the option to skip checks" entails?

Also this PR would bring JSON-RPC in line with how GraphQL works. https://github.com/MystenLabs/sui/blob/main/crates/sui-graphql-rpc/src/types/query.rs#L179 there appears to be an DevInspect.into() -> DryRunResponse so it might be good to do the swap to devinspect call in the api layer?

My first reaction to this news is that we have done a very mean thing to our GraphQL users, but I would be interested in learning more (especially with this "the option to skip checks" thing I don't understand)

My first reaction to this news is that we have done a very mean thing to our GraphQL users, but I would be interested in learning more (especially with this "the option to skip checks" thing I don't understand)

In GraphQL, there's a single combined dryRunTransactionBlock query, that combines the features that we used to offer via dry-run and dev-inspect:

• It can accept a TransactionData or a TransactionKind.
• It has a skipChecks flag which allows for running the transaction without the normal ownership checks etc (like how dev inspect works).
• Owing to how GraphQL works, users can opt in to receiving the intermediate results of commands or not.

There are some more details in the RPC 2.0 RFC: #13700, particularly this part.

We did this because people were regularly confused by the difference between dev-inspect and dry-run, and this was made worse by the fact that there were configurations that didn't work, that we did not have great reasons for, like:

• Why do we not allow people to dev inspect a transaction if we provide it as TransactionData?
• Why can't we inspect intermediate results of commands with checks enabled?
• Why can't we simulate a transaction kind, but again, with checks enabled?

The commonality is that none of these transactions' effects are committed to the chain, so we ended up retaining the dryRun nomenclature. In the end, this will probably evolve one more time into resolve/simulate, which addresses another issue of needing to use multiple round trips before a transaction is ready, and I think the naming is more standard as well, so hopefully less confusing.

there appear to be a few related to graphql.

Running tests/cli_tests.rs (target/debug/deps/cli_tests-50a62d76257781da)
test_dry_run: test

Running unittests src/lib.rs (target/debug/deps/sui_core-c73fbdd212dad4b5)
authority::authority_tests::test_dry_run_dev_inspect_dynamic_field_too_new: test
authority::authority_tests::test_dry_run_dev_inspect_max_gas_version: test
authority::authority_tests::test_dry_run_no_gas_big_transfer: test
authority::authority_tests::test_dry_run_on_validator: test
authority::authority_tests::test_dry_run_transaction_block: test
authority::authority_tests::test_for_inc_201_dry_run: test

Running tests/traffic_control_tests.rs (target/debug/deps/traffic_control_tests-894638d11d538e15)
test_fullnode_traffic_control_dry_run: test
test_validator_traffic_control_dry_run: test

**Running unittests src/lib.rs (target/debug/deps/sui_graphql_rpc-04aacd5897d6fa6e)**
server::builder::tests::test_payload_dry_run_exceeded: test
server::builder::tests::test_payload_inline_fragment_dry_run_exceeded: test
server::builder::tests::test_payload_multiple_dry_run_exceeded: test
server::builder::tests::test_payload_named_fragment_dry_run_exceeded: test
server::builder::tests::test_payload_reusing_vars_dry_run: test
server::builder::tests::test_payload_using_vars_dry_run_exceeded: test
server::builder::tests::test_payload_using_vars_dry_run_read_exceeded: test

Running tests/e2e_tests.rs (target/debug/deps/e2e_tests-74e3ae0c9e805fc2)
test_dry_run_failed_execution: test
test_transaction_dry_run: test
test_transaction_dry_run_with_kind: test

Running tests/balance_changes_tests.rs (target/debug/deps/balance_changes_tests-e4dd84a09e4c3d45)
test_dry_run_publish_with_mocked_coin: test

there appear to be a few related to graphql.

I don't think those tests are stressing the semantic difference between dry-run and dev-inspect though, which is my concern with the change in this PR.

Yeah, I don't think we have tests that would expose exactly this kind of dry-run vs dev-inspect strangeness in GraphQL (Today it's a little tough to write GraphQL transactional tests that involve building transactions to execute through GraphQL's dryRunTransactionBlock/executeTransactionBlock, so we are light there)

It would probably be better to have those tests closer to the authority codebase where those differences are maybe more apparent? (In GraphQL we are already living a post-dryRun world too).

It would probably be better to have those tests closer to the authority codebase where those differences are maybe more apparent? (In GraphQL we are already living a post-dryRun world too).

I was imagining having the tests in sui-adapter-transactional-tests, yeah.

in sui-core/src/unit_tests/authority_tests.rs

existing

• test_dry_run_no_gas_big_transfer : gas testing which I believe is to ensure you can dry run to check gas requirements
• test_dry_run_dev_inspect_max_gas_version: is an example of a successful TX against dry

I added

• test_dry_run_no_arbitrary_functions: covers both a failed dry run and one which fails from calling a private function
• test_dry_run_invalid_object_ownership: checks you cannot violate ownership during dry run, I think this covers arbitrary args via bytes? it definitely covers
• test_dry_run_publish

I'm going to sift through sui-adapter-transactional-tests some of these tests might make more sense there.

My main questions now are:
other than breaking ownership what is the "via bytes" part of arbitrary args? is this related to arbitrary values flag on mode?
https://github.com/MystenLabs/sui/blob/main/sui-execution/latest/sui-adapter/src/execution_mode.rs#L209
I was able to test this flag with test_dry_run_invalid_object_ownership.

reference return values I think are pretty straight forward specifically checking for this I think:

public fun create_and_return_ref(): &u64 {
    let s = 0;
    &s
}

while I do see the flag for conservation checks and understand what it does, how you are suppose to violate it in a test I'm not sure of. I figure you need to break a few things in core parts of the code before that check comes into play since that would mean you were able to write a tx that inflates sui right? Can't find any examples of this in the code.